-
-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: Form example for demo #1188
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
ok it's ready for review now. Sorry for all the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we can improve things later, the last comment blocks me to merge immediately.
// simulate a slow server response | ||
await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
message = `Hello ${formData.get('name') || 'Anonymous'} from server!`; | ||
unstable_rerenderRoute('/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to have this in this example, but it makes me wonder if this example is more than just "form demo".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you rename it to? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No immediate ideas. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"form demo" seems fitting to me I think. rerenderRoute
can be used for other things, but progressive forms will be a very normal use of it.
// module state on server | ||
let message = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe as a "demo", we may want to write it a file, instead of a module variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good suggestion! I added a file with some text. Let me know what you think of it :)
<Form message={getMessage()} greet={greet} /> | ||
<Form message={await getMessage()} greet={greet} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, this change is very different. I think it's intentional to not have await
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was seeing errors in the e2e tests without this because it was attempting to render the unawaited promise which threw an error.
could you checkout the first commit of this PR and see if there's a different fix I should do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it didn't cause issue with 36_form I was just matching the behavior after seeing issues with it in the demo, but I am happy to revert just 36_form if you think it is still correct after checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: Objects are not valid as a React child (found: object with keys {page:/, layout:/, root, route:/, ROUTE, IS_STATIC}). If you meant to render a collection of children, use an array instead.
It feels like a bug somewhere. But, I can't spot it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like the object that we use here: https://github.com/dai-shi/waku/blob/main/packages/waku/src/router/client.ts#L477-L481
So maybe we are crossing paths with the promise for the Router and the getMessage promise? idk how such a thing would happen though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if it's helpful. It's reproducible locally on the first load of the / route after starting the dev server. Then you need to restart the server to see the issue after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we disable SSR, the error won't happen. So, I assume it's SSR related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spent the whole day and just found out how to reproduce the error without waku/router.
diff --git a/examples/36_form/src/components/App.tsx b/examples/36_form/src/components/App.tsx
index afb38d4f..87934076 100644
--- a/examples/36_form/src/components/App.tsx
+++ b/examples/36_form/src/components/App.tsx
@@ -15,8 +15,8 @@ const App = ({ name }: { name: string }) => {
>
<h1>Hello {name}!!</h1>
<h3>This is a server component.</h3>
- <Counter increment={increment} />
<Form message={getMessage()} greet={greet} />
+ <Counter increment={increment} />
<ServerForm />
</div>
</body>
This ☝️ reproduces the error with 36_form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
adding a more user ready version of a progressive forms example